Skip to content

Pkcs11 soft ocsp tests#8557

Open
krishnavema wants to merge 2 commits intoSSSD:masterfrom
krishnavema:pkcs11-soft-ocsp-tests
Open

Pkcs11 soft ocsp tests#8557
krishnavema wants to merge 2 commits intoSSSD:masterfrom
krishnavema:pkcs11-soft-ocsp-tests

Conversation

@krishnavema
Copy link
Copy Markdown
Contributor

@krishnavema krishnavema commented Mar 27, 2026 •
edited
Loading

gemini-code-assist[bot]
gemini-code-assist Bot reviewed Mar 27, 2026
View reviewed changes
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request addresses RHEL-5043 by improving OCSP timeout handling in p11_child. It introduces a default timeout for soft_ocsp requests to prevent indefinite blocking and adjusts the OCSP deadline calculation to use half of the total allocated timeout, providing a buffer for result processing. Additionally, a new suite of system tests has been added to verify smart card authentication behavior under various OCSP responder availability scenarios. I have no feedback to provide.

@krishnavema krishnavema mentioned this pull request Mar 27, 2026
Merged
@krishnavema krishnavema requested review from spoore1 and thalman March 27, 2026 16:12
@thalman thalman self-assigned this Mar 31, 2026
@krishnavema krishnavema marked this pull request as ready for review April 1, 2026 03:55
@alexey-tikhonov
Copy link
Copy Markdown
Member

alexey-tikhonov commented Apr 9, 2026

@krishnavema, @spoore1, what branches does this target?

thalman
thalman requested changes Apr 9, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@thalman thalman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tests and change looks good, but the requirements should not point to private fork

Comment thread src/tests/system/requirements.txt
git+https://github.com/next-actions/pytest-tier
git+https://github.com/next-actions/pytest-output
git+https://github.com/SSSD/sssd-test-framework
#git+https://github.com/SSSD/sssd-test-framework
Copy link
Copy Markdown
Contributor

@thalman thalman Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This must be reverted before merging

spoore1
spoore1 requested changes Apr 16, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@spoore1 spoore1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main thing I think is to move the tests into the existing test_smartcard.py file and use it's helper functions. Besides that, mostly questions for clarification.

Comment thread src/p11_child/p11_child_openssl.c Outdated
Comment thread src/p11_child/p11_child_openssl.c Outdated
Comment thread src/tests/system/tests/test_smartcard_soft_ocsp.py Outdated
Comment thread src/tests/system/tests/test_smartcard_soft_ocsp.py Outdated
Comment thread src/tests/system/tests/test_smartcard_soft_ocsp.py Outdated
time.sleep(VIRT_CACARD_SETTLE_SECONDS)


def _assert_smartcard_auth_success(client: Client, username: str) -> None:
Copy link
Copy Markdown
Contributor

@spoore1 spoore1 Apr 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't you write an authentication util for su for this in another PR? I think that should be used here.

Copy link
Copy Markdown
Contributor Author

@krishnavema krishnavema Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess there is no existing util , do you mean somewhere in specific ?

Copy link
Copy Markdown
Contributor

@spoore1 spoore1 Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was talking about what you did in this other PR here:
https://github.com/SSSD/sssd-test-framework/pull/239/changes#diff-3399c4ca6a52ee78cd188bf544a4db3318c8e89d60f0476f0566097b58145b08

I think that should be used for this test too if it can be. Maybe we need to review that PR along with this one too?

thalman
thalman requested changes Apr 17, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@thalman thalman left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scott's questions made me go through the code once more and I think that this is bad approach. See my comment.

Comment thread src/p11_child/p11_child_openssl.c
@krishnavema
Add default timeout handling for soft_ocsp and regression tests for s…
49f5b90 
…mart card authentication (resolves: RHEL-5043)
@krishnavema krishnavema force-pushed the pkcs11-soft-ocsp-tests branch from 8845bce to 49f5b90 Compare April 17, 2026 16:50
@krishnavema krishnavema requested review from spoore1 and thalman April 17, 2026 16:59
@spoore1
Copy link
Copy Markdown
Contributor

spoore1 commented Apr 21, 2026

Testing on Fedora 42 with a downgraded version of sssd does reproduce the issue but, I'm unable to reproduce it anywhere else at this time.

[root@client ~]# rpm -q sssd
sssd-2.10.2-3.fc42.x86_64

[root@client ~]# su - smartcarduser1 -c 'su - smartcarduser1 -c whoami'
su: warning: cannot change directory to /home/smartcarduser1: No such file or directory
Password:

When I upgrade to the latest available in Fedora 42, I see it work:

[root@client ~]# rpm -q sssd
sssd-2.11.1-2.fc42.x86_64

[root@client ~]# su - smartcarduser1 -c 'su - smartcarduser1 -c whoami'
su: warning: cannot change directory to /home/smartcarduser1: No such file or directory
PIN for smartcarduser1: 
su: warning: cannot change directory to /home/smartcarduser1: No such file or directory
smartcarduser1

Testing with the version built from this PR is not working for me however:

[root@client ~]# rpm -q sssd
sssd-2.13.0-99.20260417165143281553.pr8557.161.g0b090a236.fc42.x86_64

[root@client ~]# su - smartcarduser1 -c 'su - smartcarduser1 -c whoami'
su: warning: cannot change directory to /home/smartcarduser1: No such file or directory
Password:

FYI, to test with the version built here, I used:

dnf copr enable packit/SSSD-sssd-8557 fedora-42-x86_64

Also, I tested with the master branch from sssd-ci-containers to run the Fedora 42 containers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thalman thalman Awaiting requested review from thalman

@spoore1 spoore1 Awaiting requested review from spoore1

1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Requested changes must be addressed to merge this pull request.

Assignees

@thalman thalman

@spoore1 spoore1

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@krishnavema @alexey-tikhonov @spoore1 @thalman